Implement section attribute#22719
Conversation
|
Thanks for your pull request and interest in making D better, @rikkimax! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#22719" |
62be678 to
b9deb45
Compare
|
I don't understand the c++ interop tests failing. As far as I know this shouldn't be messing with it. Windows known. I've added some debug messages to figure out why my test is failing. |
b9deb45 to
b5f0610
Compare
|
ELF is passing.
dmd/compiler/src/dmd/backend/mscoffobj.d Line 1404 in a807811 Disabled assert, works fine. Seems its more a in progress assert during some development spree and isn't an actual limitation of the code. Now passing requires the $M suffix.
|
b5f0610 to
762982c
Compare
|
|
762982c to
165f324
Compare
compiler/src/dmd/declaration.d
Outdated
| Expression edtor; // if !=null, does the destruction of the variable | ||
| IntRange* range; // if !=null, the variable is known to be within the range | ||
|
|
||
| string userDefinedSection; // the @section("name"), if present |
There was a problem hiding this comment.
Why is a field needed for this when gdc/ldc do just fine without?
(And the worst kind of field for C++ back-ends)
There was a problem hiding this comment.
It does not.
This is how opend has it implemented, and this was based upon that (but I keep finding "fun things" wrong with it).
There was a problem hiding this comment.
Processing during semantic is debatable as Udas are preserved for the codegen pass.
There was a problem hiding this comment.
Indeed, I'm happy to move it. Just haven't done it quite yet. Hopefully it clears up the c++ interop issues, I still have no idea what is causing them.
There was a problem hiding this comment.
Just duplicates information already kept elsewhere, so you're increasing the memory size of all variable/parameter/bitfields by 16 bytes for something 99.99(many 9's later)% of them won't use.
I've just realised that this is only putting global vars into custom sections. Is ignoring functions intentional?
There was a problem hiding this comment.
Ignoring functions is more like I can't be bothered to come up with a way to verify it in CI.
Realistically people are not likely to use that for dmd.
There was a problem hiding this comment.
It's pretty hard to use something that doesn't work, yes.
There was a problem hiding this comment.
If only there was a tool that could do object dumps and that same tool can display information for a given section name. :-)
There was a problem hiding this comment.
Yeah I was using objdump for testing this.
But no, I don't want to mess around with the CI and how to verify it.
a6d8c85 to
94cb8f1
Compare
|
What is the purpose of section attributes? |
| // GDC and LDC declare this attribute in their own modules. | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
documentation starts here
Placing symbols in specific sections. Very useful for embedded applications. I personally use it to guarantee that structures appear at the correct location in ROM images (though the linker does most of the heavy lifting for this). |
87507e0 to
f1ea646
Compare
f1ea646 to
8bd9ad1
Compare
|
When incrementally linking with MSVC's link it'll add padding before merging. As such, on Windows the type of the variable should be pointers: https://devblogs.microsoft.com/oldnewthing/20190114-00/?p=100695 If you use Unfortunately this means that linker lists are going to need some more thinking. |
00829e4 to
ae3f223
Compare
8e46945 to
2b0c683
Compare
|
I have updated the spec, to include that |
|
I am happy with current state. |
2b0c683 to
86435de
Compare
thewilsonator
left a comment
There was a problem hiding this comment.
Presumably Clang/GCC have the ability to do this too, can you add a C++ test that checks we match what they do?
86435de to
2912f11
Compare
|
I've gone ahead and removed the class detection for mutability. Class references (which are what a global variable stores) can be in read-only memory. Its the class instance that can't be, which this doesn't affect. |
2912f11 to
d3175b7
Compare
Upstreaming this from ldc/gdc/opend.
However I did have to reimplement the flags. They were not correct according to Gemini, and from what I can tell, it is correct.